-
Notifications
You must be signed in to change notification settings - Fork 342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add IBC fees to IbcMsg::Transfer and ::SendPacket #1749
base: main
Are you sure you want to change the base?
Conversation
f74623e
to
a3994c5
Compare
a3994c5
to
1193666
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start! Some comments on expert vs simple use cases.
/// [ICS-29]: https://github.com/cosmos/ibc-go/blob/v7.2.0/docs/middleware/ics29-fee/overview.md | ||
/// [ibc.applications.fee.v1.Fee]: https://github.com/cosmos/ibc-go/blob/v7.2.0/proto/ibc/applications/fee/v1/fee.proto#L11-L31 | ||
#[derive(Serialize, Deserialize, Clone, Debug, Default, PartialEq, Eq, JsonSchema)] | ||
pub struct IbcFee { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO This type is good for an advanced devs but I would consider it too complex for default devs. People would need to understand IBC to make proper use of these attributes. As in #1491 (comment) proposed, let's have a single fee field and split the amount in wasmd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People would need to understand IBC to make proper use of these attributes
That's an interesting point. When we are targeting IbcMsg::SendPacket users they better know this. However, IbcMsg::Transfer users do not need to know.
let's have a single fee field and split the amount in wasmd
We can go that route, yeah. However, this assumes wasmd can make better decisions than contract developers here. And right now I don't see how this can be done other than hardcode a split, which we can as well hardcode in cosmwasm-std.
pub ack: Vec<Coin>, | ||
/// The packet timeout fee | ||
pub timeout: Vec<Coin>, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it makes sense to pass an optional list of relayer addresses here, too so that the contract is in control who can receive incentives for relaying. This may be an advanced use case but should be covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it out in order to keep this type simple. A developer can access to full API by adding a MsgPayPacketFee instead of using this field. Do we really need it here?
No description provided.